Stabilize is_symlink() for Metadata and Path - #89677

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rfcbot merge

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

Concerns:

• should-return-result resolved by Stabilize is_symlink() for Metadata and Path #89677 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

I'm not fully sure about since version, correct me if I'm wrong

Its accurate provided this PR lands before sometime in October 21st.

Shouldn't this return Result in the spirit of try_exists? Or is it considered too inconsistent given existing methods return bool?

@Kixunil Good catch!

@rfcbot concern should-return-result

Shouldn't this return Result in the spirit of try_exists? Or is it considered too inconsistent given existing methods return bool?

I don't think it should. There are is_dir and if_file methods with this signature (stable since 1.5.0). This question is also discussed in #76487 and #83186.

Personally, I think it would be inconvenient to return Result in this method, but it might be useful to have try_exists_symlink to some extend, but to what extent?

For now it works well playground

@Kixunil @joshtriplett

I personally think that bools are footguns here - easy to forget about the case that syscall may fail. It does make sense on Metadata, which already performed such check though.

The question is will we deprecate exists&co eventually? If so stabilizing this just to deprecate it later doesn't sound well.

? is one-character operator and if you can't be bothered to adjust error types because you don't believe it will ever be error, just unwrap is better for debugging.

I agree that on Metadata we can just return bool. On Path, I think we should return a result.

@joshtriplett, Wouldn't it be illogical to have is_dir() -> bool and is_file() -> bool, but is_symlink() -> Result<bool, Error>?

I see 3 ways to solve this:

1. Deprecate is_dir and is_file -> bool signatures and replace these with try_is_.... This will brake A LOT of code and make these methods so inconviniet to use due to error handling. These methods alwo used in functions that retuns a lot of types of errors, so you can't just use ?, also you can't use .unwrap() because it will be strange to see in production code, so you will use .unwrap_or(false), as currently implemented. A programmer who wants to handle methadata error can do it by himself.
2. Accept this PR to comlete a "method hole" and continue thinking about it.
3. Accept this PR and open new, not deprecating old methods. New PR will add try_is..., so it will be convinient to use for everyone.

make these methods so inconviniet to use

It's only inconvenient if you intend to write dubious code that silently ignores possibly serious issue. Rust is generally designed to not ignore errors.

also you can't use .unwrap() because it will be strange to see in production code, so you will use .unwrap_or(false)

If you can't use unwrap() in production you can't use unwrap_or(false) either, that's just silently swallowing possibly serious issue. Actually I'd rather use unwrap() in production than implicit unwrap_or(false) because that way I at least learn that something is wrong. Silent errors are super-nightmare of debugging.

Given that it'd be strange that there are methods with is_ that return bool it makes most sense to me to add remaining try_ methods and rename this one to begin with try_.

@Kixunil I agree with you. So we change is_symlink to try_is_symlink and add in new PR try_ versions for is_file and is_dir? What about deprecation?

Does try_is_* provide enough value over try { metadata()?.is_*() } to warrant the method? Not to mention that metadata version naturally directs users towards reuse of the metadata query in case they want to check for multiple things (e.g. symlink || file)

@maxwase that's what I believe would be the best. Not sure about deprecation but I would be in favor of it once try_* methods are in std long enough.

@nagisa sure, I think simple method is much more likely to stabilize than try { } block. ;)

try is quite likely addition to the language AFAIK. If we stabilized try within a year's time, is the value provided by such a method over this period really all that high to warrant us dealing with its existence for the remaining lifetime of the language?

My personal opinion is that we should either stabilize Path::is_symlink as is for consistency with Path::is_file and Path::is_dir; or we shouldn't add any function to Path at all, direct users towards metadata and wait for try {} to improve ergonomics somewhat.

I think I'm in agreement with @nagisa and @maxwase. I don't believe it would be a good idea to add a Result returning function here to Path directly. All of the related methods on Path have documentation pointing users towards metadata() when one wants to handle errors explicitly, separate from the boolean return from is_file and friends. I think in this case we should be prioritizing internal consistency with other related Path APIs over fears of error-proneness. If we think that these helper APIs that return bools are particularly error-prone then that is a separate issue and we should deprecate them and universally push users towards metadata and symlink_metadata, not introduce a third way to call the same API.

That said, while all of the other methods on Path have a note indicating what API to use when explicit error handling is required, is_symlink does not. I would like to see a similar section added to the documentation for is_symlink to point users towards the fs::symlink_metadata method when explicit error handling is required.

I do personally feel that we should have try versions of these methods, but that doesn't need to be a blocker here.

@rfcbot resolved should-return-result

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@bors r+

📌 Commit 3e0360f has been approved by joshtriplett

I don't fully grasp the state of this feature; isn't it available as of 1.57?

I'm getting an error indicating that it's unstable:

error[E0658]: use of unstable library feature 'is_symlink'
  --> src/main.rs:82:22
   |
82 |         if home_path.is_symlink() {
   |                      ^^^^^^^^^^
   |
   = note: see issue #85748 <https://github.com/rust-lang/rust/issues/85748> for more information

error[E0658]: use of unstable library feature 'is_symlink'
  --> src/main.rs:87:26
   |
87 |     } else if !home_path.is_symlink() {
   |                          ^^^^^^^^^^
   |
   = note: see issue #85748 <https://github.com/rust-lang/rust/issues/85748> for more information

If you look at the sidebar you should see a "milestone" tag. This says it'll be stable in 1.58.

Ah, that completely slipped past me, thanks!

No worries. It's easy to miss if you don't know to look for it.

I don't fully grasp the state of this feature; isn't it available as of 1.57?

I'm getting an error indicating that it's unstable:

error[E0658]: use of unstable library feature 'is_symlink'
  --> src/main.rs:82:22
   |
82 |         if home_path.is_symlink() {
   |                      ^^^^^^^^^^
   |
   = note: see issue #85748 <https://github.com/rust-lang/rust/issues/85748> for more information

error[E0658]: use of unstable library feature 'is_symlink'
  --> src/main.rs:87:26
   |
87 |     } else if !home_path.is_symlink() {
   |                          ^^^^^^^^^^
   |
   = note: see issue #85748 <https://github.com/rust-lang/rust/issues/85748> for more information

I don't understand this too. Stable attribute is 1.57 in master, but milestone is 1.58 here. Also this PR was not mentioned in release list
@joshtriplett, can you please explain?

it means the #[stable] version property is wrongly tagged 🤷

it means the #[stable] version property is wrongly tagged 🤷

Should I replace it in new PR?

yes